packages/features/insights/server/trpc-router.ts (2)

890-895: Preserve error context or add logging for debuggability.

Catching and rethrowing INTERNAL_SERVER_ERROR loses the root cause. Consider logging the original error (with safe metadata) or attaching a minimal message for observability.

Apply this small diff:

-      try {
-        return await insightsRoutingService.getFailedBookingsByFieldData();
-      } catch (e) {
-        throw new TRPCError({ code: "INTERNAL_SERVER_ERROR" });
-      }
+      try {
+        return await insightsRoutingService.getFailedBookingsByFieldData();
+      } catch (e) {
+        ctx.logger?.error?.({ err: e, route: "viewer.insights.failedBookingsByField" }, "failedBookingsByField error");
+        throw new TRPCError({ code: "INTERNAL_SERVER_ERROR" });
+      }

---

888-896: (Optional) Guardrail: enforce presence of routing form filter at the edge.

UI currently guarantees a routingFormId, but adding a lightweight guard prevents accidental regressions that could reintroduce the “all forms” bug.

Proposed diff:

   .query(async ({ ctx, input }) => {
     const insightsRoutingService = createInsightsRoutingService(ctx, input);
+    const hasFormIdFilter =
+      (input.columnFilters || []).some((f) => f.id === "formId" && !!(f as any).value?.data);
+    if (!hasFormIdFilter) {
+      // Defensive: UI should always pass a formId, keep server strict.
+      throw new TRPCError({ code: "BAD_REQUEST", message: "routingFormId is required" });
+    }
     try {
       return await insightsRoutingService.getFailedBookingsByFieldData();
     } catch (e) {
       throw new TRPCError({ code: "INTERNAL_SERVER_ERROR" });
     }
   })

packages/features/insights/components/routing/FailedBookingsByField.tsx (1)

134-136: Pass-through of insightsRoutingParams to TRPC matches the new endpoint; consider enabling only when formId exists.

The query call is correct. As a minor guard, you could enable the query only when the hook has emitted a formId to avoid any early, empty fetch cycles.

Optional diff:

-  const { data } = trpc.viewer.insights.failedBookingsByField.useQuery(insightsRoutingParams);
+  const enabled = (insightsRoutingParams.columnFilters || []).some((f) => f.id === "formId");
+  const { data } = trpc.viewer.insights.failedBookingsByField.useQuery(insightsRoutingParams, { enabled });

packages/lib/server/service/InsightsRoutingBaseService.ts (3)

436-437: Qualifying filters with rfrd. is correct; minor robustness tweaks possible.*

• The aliasing fixes are spot-on and ensure filters hit the denormalized source consistently.
• For bookingUserId ANY, consider coercing to numeric to avoid accidental text[] bindings from UI.

Proposed diff:

-      conditions.push(Prisma.sql`rfrd."bookingUserId" = ANY(${memberUserIds.value.data})`);
+      conditions.push(Prisma.sql`rfrd."bookingUserId" = ANY(${memberUserIds.value.data.map(Number)})`);

If bookingUserId filter values can be strings, this prevents a Postgres type mismatch. If they’re guaranteed numbers upstream, feel free to skip.

Also applies to: 453-454, 462-463, 471-472, 505-506, 513-514

---

760-796: Sort options within each field for clearer UX; ORDER BY in SQL is redundant post-aggregation.

• After grouping, options per field aren’t sorted; sort descending by count to show most frequent failures first.
• Since you re-aggregate in TS, the SQL ORDER BY count DESC is not required; removing it can save the planner unnecessary work.

Apply this diff:

-    const groupedByFormAndField = result.reduce((acc, curr) => {
+    const groupedByFormAndField = result.reduce((acc, curr) => {
       const formKey = curr.formName;
       acc[formKey] = acc[formKey] || {};
       const labelKey = curr.fieldLabel;
       acc[formKey][labelKey] = acc[formKey][labelKey] || [];
       acc[formKey][labelKey].push({
         optionId: curr.optionId,
         count: curr.count,
         optionLabel: curr.optionLabel,
       });
       return acc;
     }, {} as Record<string, Record<string, { optionId: string; count: number; optionLabel: string }[]>>);
+
+    // Sort options within each field by count desc for better readability
+    for (const fields of Object.values(groupedByFormAndField)) {
+      for (const label of Object.keys(fields)) {
+        fields[label] = fields[label].sort((a, b) => b.count - a.count);
+      }
+    }

Optionally remove the SQL ORDER BY:

-    WHERE ff.option_id IS NOT NULL
-    ORDER BY count DESC
+    WHERE ff.option_id IS NOT NULL

---

697-796: Keying by formName risks collisions if multiple forms share the same name (when formId filter is absent).

UI currently enforces a routingFormId, so you’re safe. If that ever changes, consider keying by formId and passing formName alongside to avoid conflation.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.

Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.329Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.

Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/ai/_router.ts:46-84
Timestamp: 2025-08-14T10:48:52.586Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/ai/_router.ts, the voiceId input parameter in the create endpoint is intentionally not forwarded to aiService.createAgent() as voice customization is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.

Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/lib/server/service/insightsRouting.ts:367-368
Timestamp: 2025-07-15T13:02:17.403Z
Learning: In the InsightsRoutingService (packages/lib/server/service/insightsRouting.ts), multi-select filter data is already validated by zod before reaching the buildFormFieldSqlCondition method, so null/undefined values are not present in filterValue.data arrays and don't need to be filtered out.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.

Learnt from: eunjae-lee
PR: calcom/cal.com#23166
File: packages/prisma/migrations/20250818151914_routing_form_response_denormalized_backfill2/migration.sql:65-66
Timestamp: 2025-08-19T09:47:49.478Z
Learning: The Booking table has a unique constraint and index on the uid column (defined as `uid String unique` in schema.prisma), so JOINs on Booking.uid have optimal performance and don't require additional indexing.

packages/features/insights/server/trpc-router.ts (1)

888-896: Endpoint refactor to InsightsRoutingService looks correct; input contract aligned.

• Switching to insightsRoutingServiceInputSchema and delegating to createInsightsRoutingService(ctx, input).getFailedBookingsByFieldData() matches the new service API and fixes the legacy coupling. Looks good.

packages/features/insights/components/routing/FailedBookingsByField.tsx (1)

19-19: Hook swap to useInsightsRoutingParameters is aligned with the new backend input shape.

The import change is correct and consistent with the TRPC schema refactor.

packages/lib/server/service/InsightsRoutingBaseService.ts (2)

566-566: Authorization parity with rfrd aliasing looks right; validate scope semantics against legacy behavior.

• user: rfrd.formUserId = userId AND rfrd.formTeamId IS NULL
• org: teamIds OR user’s personal forms
• team: exact teamId

This matches expectations for routing responses. Please double-check any edge cases where orgId may be null under “user” scope so validation doesn’t null out options.

Would you like a small unit-test harness for getAuthorizationConditions across scopes?

Also applies to: 587-587, 603-604

---

697-759: CTE accurately computes failed bookings by form/field/option; zero-count options preserved.

• Good: baseConditions applied; failed defined as bookingUid IS NULL; DISTINCT response counting prevents duplicates; options expanded from form JSON and left-joined to include zero-counts.
• This directly addresses “selected form only” when formId is present in columnFilters.

packages/lib/server/service/__tests__/InsightsRoutingService.integration-test.ts (6)

912-915: Enum source-of-truth for bookingStatusOrder and bookingUserId filters.

The tests use string literals (e.g., "pending", "accepted"). If bookingStatusOrder maps to an enum, prefer referencing the enum constants in test construction to catch drift early. For user IDs, ensure the ANY array is typed (see earlier comment).

If bookingStatusOrder is an enum, consider:

• Using BookingStatus.PENDING/ACCEPTED (if values match), or
• A mapping helper to translate enum to stored string in the denormalized view.

Also applies to: 1029-1032

---

1067-1076: Subqueries for attendee filters: correctness good; ensure indexes for performance.

The EXISTS subqueries joining Booking and Attendee look correct and safe (parameterized ILIKE). For production scale, confirm indexes exist on:

• Attendee.bookingId
• Attendee.name, Attendee.email, Attendee.phoneNumber (consider trigram or lower(text) indexes if these predicates are hot)

Would you like me to propose index DDLs based on current query patterns?

Also applies to: 1111-1120, 1155-1164

---

1247-1255: Multi-select semantics: confirm “match all” (@>) vs “overlap any” (&&).

Using @> means the response must contain all selected options. If the UI’s multi-select filter is intended as “any of” semantics, use the overlap operator (&&) instead.

If “any-of” is desired, adapt the expectation to:

-AND rrf."valueStringArray" @> ${["option1", "option2"]}
+AND rrf."valueStringArray" && ${Prisma.sql`ARRAY[${Prisma.join(["option1","option2"])}]`}::text[]

And mirror the same in the service implementation.

---

1306-1316: Brittle parentheses in combined condition expectations.

The nested parentheses make tests sensitive to harmless formatting changes in the builder. Consider composing with intermediate fragments and a normalizer or assert via substring presence of each conjunct.

Example approach inside the test:

• Build expected parts individually (date, status, reason, custom field).
• Combine via Prisma.sql${part1} AND ${part2} AND ${part3} to reduce nesting depth.

---

90-104: Make field IDs consistent between form definition and response payload.

You generate random IDs for fields, then use different random IDs as keys in the response. This works for current tests (as they don’t assert field-level joins here) but can break future tests or denormalization assumptions. Reuse the same IDs for both.

Apply this refactor:

@@
-  const form = await prisma.app_RoutingForms_Form.create({
+  const textFieldId = `text-field-id-${randomUUID()}`;
+  const emailFieldId = `email-field-id-${randomUUID()}`;
+  const form = await prisma.app_RoutingForms_Form.create({
     data: {
       name: "Test Routing Form",
       userId: user.id,
       teamId: team.id,
       fields: [
         {
-          id: `text-field-id-${randomUUID()}`,
+          id: textFieldId,
           type: "text",
           label: "Name",
           required: true,
         },
         {
-          id: `email-field-id-${randomUUID()}`,
+          id: emailFieldId,
           type: "email",
           label: "Email",
           required: true,
         },
       ],
     },
   });
@@
-  const formResponse = await prisma.app_RoutingForms_FormResponse.create({
+  const formResponse = await prisma.app_RoutingForms_FormResponse.create({
     data: {
       formFillerId: `test-filler-${randomUUID()}`,
       formId: form.id,
       response: {
-        [`text-field-id-${randomUUID()}`]: {
+        [textFieldId]: {
           label: "Name",
           value: "Test Name",
         },
-        [`email-field-id-${randomUUID()}`]: {
+        [emailFieldId]: {
           label: "Email",
           value: "test@example.com",
         },
       },
       routedToBookingUid: booking.uid,
     },
   });

Also applies to: 113-121

---

249-257: Broaden coverage: add tests for getFailedBookingsByFieldData() output shape.

This file thoroughly tests auth/filter fragments. Given this PR introduces getFailedBookingsByFieldData(), consider adding at least one integration test asserting the nested form → field → option counts shape and date filtering respects rfrd aliasing.

I can draft a minimal test that:

• Creates one form with two fields and two responses, one failed (no routedToBookingUid), one successful.
• Calls service.getFailedBookingsByFieldData() with routingFormId filter and date bounds.
• Asserts aggregation counts and that only the selected form is included.
  Would you like me to open a follow-up PR with this test?

Also applies to: 516-544

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.

Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/RoutingFunnel.tsx:15-17
Timestamp: 2025-07-15T12:58:40.539Z
Learning: In the insights routing funnel component (packages/features/insights/components/RoutingFunnel.tsx), the useColumnFilters exclusions are intentionally different from the general useInsightsParameters exclusions. RoutingFunnel specifically excludes only ["createdAt"] while useInsightsParameters excludes ["bookingUserId", "formId", "createdAt", "eventTypeId"]. This difference is by design.

Learnt from: bandhan-majumder
PR: calcom/cal.com#23192
File: packages/lib/server/service/InsightsBookingBaseService.ts:814-816
Timestamp: 2025-08-22T16:38:00.182Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), when filtering for "accepted" bookings in getMembersStatsWithCount(), using `endTime <= now()` in the SQL condition should be avoided as it conflicts with existing date filtering logic. The components already handle completion filtering by setting `endDate: currentTime` in their query parameters, making additional SQL-level endTime filtering unnecessary and potentially problematic.